Skip to content

Conversation

@FireMasterK
Copy link
Member

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

This takes an alternative approach to #576

@Stypox could you verify that this approach is okay?

@FireMasterK FireMasterK marked this pull request as draft August 4, 2021 12:33
@FireMasterK FireMasterK marked this pull request as ready for review August 4, 2021 15:33
@Stypox
Copy link
Member

Stypox commented Aug 4, 2021

Does it work also with comments continuations? Would this approach allow having both nested replies (for Peertube, I think) and replies continuations (when not all replies are loaded at once)?

@FireMasterK
Copy link
Member Author

FireMasterK commented Aug 4, 2021

Does it work also with comments continuations

Yes! Nothing should break it, although I will check it manually. I would have to check this once again as I have not check if there's a different JSON structure for these continuations. Edit: Fixed

Would this approach allow having both nested replies

Yes, each comment would have an option replies Page. In YouTube there's only one level, so it would be null there.

To get reply Comments, you just call getMoreItems with the replies Page.

replies continuations

This would be a regular continuation / getNextPage(), nothing should change.

@Stypox
Copy link
Member

Stypox commented Aug 4, 2021

@FireMasterK ok, then this approach is good. Thank you :-)

@FireMasterK
Copy link
Member Author

FireMasterK commented Aug 4, 2021

Just one small issue - this implementation doesn't support multi-level threads in one page request, but I think that can be done later. (I'm not sure if any services do this either)
Edit: This is the case in Peertube

@Mhowser
Copy link

Mhowser commented Sep 7, 2021

Is this PR ready to be reviewed and merged soon?

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thank you :-D

@Stypox
Copy link
Member

Stypox commented Sep 13, 2021

This will be merged after release 0.20.10 is released

@TobiGr
Copy link
Contributor

TobiGr commented Sep 13, 2021

Do we need to squash the commits?

@Stypox
Copy link
Member

Stypox commented Sep 14, 2021

I don't think that's needed

@TobiGr TobiGr merged commit a9d2144 into TeamNewPipe:dev Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants